- 
                Notifications
    
You must be signed in to change notification settings  - Fork 594
 
Add easy initialization macros #1270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
          Codecov ReportAttention: Patch coverage is  
 
 Additional details and impacted files@@            Coverage Diff             @@
##             main    #1270      +/-   ##
==========================================
- Coverage   91.79%   91.67%   -0.13%     
==========================================
  Files          37       38       +1     
  Lines       18185    18299     +114     
==========================================
+ Hits        16693    16775      +82     
- Misses       1492     1524      +32     ☔ View full report in Codecov by Sentry.  | 
    
| 
           Another open problem: clippy warns about integers starting with   | 
    
          
 Can probably add clippy   | 
    
402fe1c    to
    0cb1204      
    Compare
  
    | 
           Fixed the clippy warning with help from @Alexendoo in rust-lang/rust-clippy#11472.  | 
    
0cb1204    to
    b1769a6      
    Compare
  
    
          
  | 
    
b1769a6    to
    c693f86      
    Compare
  
    | 
           I have separated this into smaller commits. And by creating a temporary   | 
    
c693f86    to
    972a08f      
    Compare
  
    972a08f    to
    6e3f9fc      
    Compare
  
    | 
           Updated to remove the rust 1.57 workaround. These macro's can make tests nicer to read. Example commit: 8e4092f.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are very handy!
I have some suggestions to improve user-friendliness.
        
          
                src/macros.rs
              
                Outdated
          
        
      | @@ -0,0 +1,316 @@ | |||
| //! Macro's for easy initialization of date and time values. | |||
| 
               | 
          |||
| /// Create a `NaiveDate` with a statically known value. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link the NaiveDate.
Since the macro does not have a return type the docs will not automatically link to NaiveDate.
So change to a link
 /// Create a [`NaiveDate`] with ...
at the bottom, add the link target
/// [`NaiveDate`]: crate::naive::NaiveDate
btw, if you use VS Code, it will highlight the link to turquoise blue when it is correctly set.
This nitpick goes for the rest of the PR.
| (-$h:literal:$m:literal) => {{ | ||
| #[allow(clippy::zero_prefixed_literal)] | ||
| { | ||
| assert!($h < 24 || $m < 60, "invalid offset"); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend splitting these out to be more specific what value is wrong. Also, reprint the wrong value. This helps the user quickly and easily identify the culprit.
 assert!($h < 24, "invalid hour offset {}", $h);
 assert!($m < 60, "invalid minute offset {}", $m);
Apply this comment to rest of the PR.
| { | ||
| const DATE: $crate::NaiveDate = match $crate::NaiveDate::from_ymd_opt($y, $m, $d) { | ||
| Some(d) => d, | ||
| None => panic!("invalid calendar date"), | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to repeat back to the user the invalid values. More specific clues in error messages are always nice.
 panic!("invalid calendar date from values {:?}, {:?}, {:?}", $y, $m, $d)
Apply this comment to rest of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comments, thanks @jtmoon79!
I'm not sure if we can use a format string for panics in const context, let me try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because these macro's are evaluated at compile time the message is already quite nice in my opinion:
error[E0080]: evaluation of constant value failed
  --> src\macros.rs:52:20
   |
52 |         assert_eq!(date!(2023-04-31), NaiveDate::from_ymd_opt(2023, 4, 30).unwrap());
   |                    ^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'invalid calendar date', src\macros.rs:52:20
   |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because these macro's are evaluated at compile time the message is already quite nice in my opinion:
error[E0080]: evaluation of constant value failed --> src\macros.rs:52:20 | 52 | assert_eq!(date!(2023-04-31), NaiveDate::from_ymd_opt(2023, 4, 30).unwrap()); | ^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'invalid calendar date', src\macros.rs:52:20 |
I still think a nice touch here is to be specific about precisely which argument caused the panic. Up to you.
6e3f9fc    to
    3126c6b      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. However I like my idea of getting specific about precisely which argument made the date invalid. Up to you.
| 
           I think it would be useful to have an initialization macro for  // DateTime<Utc>
datetime!(2023-09-08 7:03:25Z);
// DateTime<FixedOffset>
datetime!(2023-09-08 7:03:25+00:00); | 
    
          
 Agreed. But as I understand such a syntax, using a letter after a number, is not supported by simple macro's. And my goal was to keep it simple, without resorting to proc macro's. Note that you can do // DateTime<Utc>
datetime!(2023-09-08 7:03:25).and_utc(); | 
    
| 
           Found a way to accept a time with fractions of a second using the  So we can now also do  @djc What would be needed to push this PR forwards? I think it will be a really good addition to improve the ergonomics of chrono. b.t.w. I opened rust-lang/rust#123156 for the CI errors.  | 
    
| 
           Hey 👋 Just dropping this in case you want to use it. When the panicking APIs were deprecated we implemented an extension trait to have easy initialization. It won't solve your issue with const context but it works very well in other context 😊 use chrono::TimeDelta;
pub trait TimeDeltaExt {
	fn weeks<Int: WaySmallerThanI64>(weeks: Int) -> Self;
	fn days<Int: WaySmallerThanI64>(days: Int) -> Self;
	fn hours<Int: WaySmallerThanI64>(hours: Int) -> Self;
	fn minutes<Int: WaySmallerThanI64>(minutes: Int) -> Self;
	fn seconds<Int: WaySmallerThanI64>(seconds: Int) -> Self;
	fn milliseconds<Int: WaySmallerThanI64>(milliseconds: Int) -> Self;
}
impl TimeDeltaExt for TimeDelta {
	fn weeks<Int: WaySmallerThanI64>(weeks: Int) -> Self {
		Self::try_weeks(weeks.into())
			.expect("-(i64::MAX / 1_000) < (WaySmallerThanI64 * 7 * 24 * 60 * 60) < (i64::MAX / 1_000)")
	}
	fn days<Int: WaySmallerThanI64>(days: Int) -> Self {
		Self::try_days(days.into())
			.expect("-(i64::MAX / 1_000) < (WaySmallerThanI64 * 24 * 60 * 60) < (i64::MAX / 1_000)")
	}
	fn hours<Int: WaySmallerThanI64>(hours: Int) -> Self {
		Self::try_hours(hours.into()).expect("-(i64::MAX / 1_000) < (WaySmallerThanI64 * 60 * 60) < (i64::MAX / 1_000)")
	}
	fn minutes<Int: WaySmallerThanI64>(minutes: Int) -> Self {
		Self::try_minutes(minutes.into()).expect("-(i64::MAX / 1_000) < (WaySmallerThanI64 * 60) < (i64::MAX / 1_000)")
	}
	fn seconds<Int: WaySmallerThanI64>(seconds: Int) -> Self {
		Self::try_seconds(seconds.into()).expect("-(i64::MAX / 1_000) < WaySmallerThanI64 < (i64::MAX / 1_000)")
	}
	fn milliseconds<Int: WaySmallerThanI64>(milliseconds: Int) -> Self {
		Self::try_milliseconds(milliseconds.into()).expect("-i64::MAX < i32 < i64WaySmallerThanI64MAX")
	}
}
pub(crate) trait WaySmallerThanI64: Into<i64> {}
impl WaySmallerThanI64 for u8 {}
impl WaySmallerThanI64 for u16 {}
impl WaySmallerThanI64 for u32 {}
impl WaySmallerThanI64 for i8 {}
impl WaySmallerThanI64 for i16 {}
impl WaySmallerThanI64 for i32 {} | 
    
I am very happy to have something like this!
My macro skills are not great though. I do not know how to avoid the code duplication. And I can't make the
time!macro support fractional seconds yet. Something TT muncher something.Depends on #1069 for creating a constDateTime<FixedOffset>.Fixes #13.